Vertical coordinate updates and fixes #309
Conversation
Edit objects to fetch the EdgeMask from VertCoord and add option to not read VertCoord stream during initialization in order to simplify tests where it is not needed
Allow for option to pass a local NVertLayers value to VertCoord::init instead of reading NVertLayers from the mesh file. Simplifies initialization of some unit tests where a local NVertLayers value is used.
Check streams for fields with new Omega "Layer" naming convention and old MPAS "Level" naming convention
philipwjones
left a comment
philipwjones
left a comment
There was a problem hiding this comment.
Looks good. Just had one minor suggestion below. Also verified it passes unit tests.
There was a problem hiding this comment.
The changes look mostly good. I left a couple of small comments.
However, I ran the test suite on pm-cpu with Kokkos bounds checking turned on and got many fails. They all look like this:
1: Kokkos::View ERROR: out of bounds access label=("VertexMask") with indices [1008,16] but extents [1169,16]
Specifically the following tests failed:
13 - AUXVARS_PLANE_TEST (Failed)
14 - AUXVARS_SPHERE_TEST (Failed)
15 - AUXSTATE_TEST (Failed)
20 - TEND_PLANE_TEST (Failed)
21 - TEND_PLANE_SINGLE_PRECISION_TEST (Failed)
22 - TEND_SPHERE_TEST (Failed)
23 - TENDENCIES_TEST (Failed)
27 - TIMESTEPPER_TEST (Failed)
31 - TRACERS_TEST (Failed)
34 - EOS_TEST (Failed)
I looked into the failures a little bit and I think they might be related to how the MaxLayerCell array is set in the case of prescribed NVertLayers. Because deepCopy is used, the value of MaxLayerCell(NCellsAll) is set to NVertLayers, but then never adjusted for 0-based indexing. I am not sure if this element should be set at all, or just be zero.
I have one more suggestion related to an issue I ran into when testing my hierarchical parallelism changes. I believe that currently when the MinLayerCell and MaxLayerCell arrays are read through iostreams they do not have their halo regions filled. So even thought the computations of the other Min/Max arrays and masks are done for all elements, they do not contain valid values everywhere. @brian-oneill Do you think it makes sense to fix this as part of this PR ?
| AuxiliaryState(const std::string &Name, const HorzMesh *Mesh, Halo *MeshHalo, | ||
| int NVertLayers, int NTracers); | ||
| AuxiliaryState(const std::string &Name, const HorzMesh *Mesh, | ||
| const VertCoord *VCoord, Halo *MeshHalo, int NVertLayers, |
There was a problem hiding this comment.
Now that you passing in an instance of VertCoord you could remove the NVertLayers argument.
| const HorzMesh *Mesh, const VertCoord *VCoord, | ||
| const I4 NVertLayers, const I4 NTracers) |
There was a problem hiding this comment.
Same as in the AuxiliaryState, there is now no need to pass NVertLayers separately. This comment applies also to the other auxiliary vars that had similar changes.
| const bool ReadStream, //< [in] logical to read stream | ||
| const int InNVertLayers //< [in] int to set vertical dim |
There was a problem hiding this comment.
Is it valid to have ReadStream = true and non-zero InVertLayers ? If not, can we check for this case and abort ?
| ABORT_ERROR("VertCoord: Unknown MovementWeightType requested"); | ||
| } | ||
|
|
||
| // Fetch reference desnity from Config |
There was a problem hiding this comment.
| // Fetch reference desnity from Config | |
| // Fetch reference density from Config |
3 participants
This PR implements fixes, additions, and refactoring for the vertical coordinate module, including:
EdgeMaskfromHorzMeshtoVertCoord, and fixing initialization soEdgeMaskproperly accounts for bathymetryVertCoord, initialization is consolidated into a singleVertCoord::init()methodLOG_WARNmessages withLOG_INFOduring constructionNVertLayers,MaxLayerCell, andMinLayerCellfrom mesh fileVertCoord::init()to enhance flexibility in unit testsCellMaskandVertexMaskarrays for potential future useVertCoordunit testCtests were run successfully on Frontier CPU & GPU, pm-cpu, pm-gpu, and Chrysalis
Fixes #290
Checklist